Fix: Secure TLS for Prometheus metrics endpoint#120
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: malingatembo The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
📝 WalkthroughWalkthroughThe metrics server is updated to use OpenShift-provisioned TLS certificates instead of self-signed ones. The metrics Service is annotated to trigger OpenShift certificate generation into the ChangesMetrics Server TLS Hardening
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
config/prometheus/monitor.yaml (1)
18-19: 🎯 Functional Correctness | 🔵 Trivial | 🏗️ Heavy liftAvoid hardcoding one rendered namespace into
serverName.The rest of this manifest set still uses the
systemplaceholder namespace, but Line 19 pins hostname verification tometrics.openstack-lightspeed-system.svc. That only works for one rendered namespace; installing into any other namespace will break TLS verification against the service-ca cert. Please driveserverNamefrom the same namespace transform/replacement path as the Service resource instead of baking in a single output.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@config/prometheus/monitor.yaml` around lines 18 - 19, The Prometheus TLS config is hardcoding a rendered namespace in the serverName value, which will break hostname verification when the manifest is installed into a different namespace. Update the serverName in the Prometheus config to use the same namespace substitution/replacement mechanism as the Service resource and the rest of this manifest set, so it is generated dynamically from the target namespace instead of being fixed to metrics.openstack-lightspeed-system.svc.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/main.go`:
- Around line 113-116: The metrics server setup is missing the TLS policy from
tlsOpts, so only the serving cert paths are being applied. Update the metrics
server options in main to also set TLSOpts: tlsOpts alongside CertDir, CertName,
and KeyName so the metrics endpoint inherits the same TLS 1.3 minimum and HTTP/2
disablement as the webhook server.
---
Nitpick comments:
In `@config/prometheus/monitor.yaml`:
- Around line 18-19: The Prometheus TLS config is hardcoding a rendered
namespace in the serverName value, which will break hostname verification when
the manifest is installed into a different namespace. Update the serverName in
the Prometheus config to use the same namespace substitution/replacement
mechanism as the Service resource and the rest of this manifest set, so it is
generated dynamically from the target namespace instead of being fixed to
metrics.openstack-lightspeed-system.svc.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a9dc39c-bf0c-4d32-8c47-657fb3555182
📒 Files selected for processing (6)
cmd/main.goconfig/default/metrics_service.yamlconfig/manager/manager.yamlconfig/prometheus/ca-bundle-configmap.yamlconfig/prometheus/kustomization.yamlconfig/prometheus/monitor.yaml
malingatembo
force-pushed
the
feat/OSPR-30557_tls-metrics-endpoint
branch
from
de7f4f0 to
073deac
Compare
Remove insecureSkipVerify from ServiceMonitor and implement proper TLS certificate verification using OpenShift service-ca. Changes: - Add service-ca annotation to metrics Service for automatic cert generation - Mount certificate Secret in operator Pod - Configure metrics server to use service-ca certificates - Update ServiceMonitor with CA bundle and server name verification - Create CA bundle ConfigMap for Prometheus - Regenerate bundle manifests Fixes: OSPR-30557 On branch feat/OSPR-30557_tls-metrics-endpoint modified: bundle/manifests/openstack-lightspeed-operator-metrics_v1_service.yaml modified: bundle/manifests/openstack-lightspeed-operator.clusterserviceversion.yaml modified: cmd/main.go modified: config/default/metrics_service.yaml modified: config/manager/manager.yaml new file: config/prometheus/ca-bundle-configmap.yaml modified: config/prometheus/kustomization.yaml modified: config/prometheus/monitor.yaml
malingatembo
force-pushed
the
feat/OSPR-30557_tls-metrics-endpoint
branch
from
073deac to
845c723
Compare
1 participant
Followed pattern from
openstack-k8s-operators/telemetry-operatorCommit: 6111e80d by Jaromir Wysoglad (2024-02-28)
TLS implementation for Prometheus metrics in OpenShift
How adapted here
Jaromir's approach: Optional TLS via API (flexible, supports cert-manager or service-ca)
Our approach: Always-on TLS with service-ca only
Core pattern (5 components):
Result:
same security outcome as reference implementation
simpler
automatic certificate management
Files Changed
6 source files + 4 auto-generated bundle files (via make bundle)
On branch feat/OSPR-30557_tls-metrics-endpoint
Summary by CodeRabbit
New Features
Bug Fixes